Skip to content

actually provide the correct args to coroutine witnesses #145338

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 19, 2025

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Aug 13, 2025

#145194 accidentally provided all arguments of the closure to the witness, but the witness only takes the generic parameters of the defining scope:

let interior = Ty::new_coroutine_witness(tcx, expr_def_id.to_def_id(), parent_args);

Fixes #145288

@rustbot
Copy link
Collaborator

rustbot commented Aug 13, 2025

r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Aug 13, 2025
@lcnr
Copy link
Contributor Author

lcnr commented Aug 13, 2025

r? compiler-errors

@rustbot
Copy link
Collaborator

rustbot commented Aug 13, 2025

compiler-errors is not on the review rotation at the moment.
They may take a while to respond.

@lcnr lcnr force-pushed the coroutine-witness-yikes branch from abde86f to 4d84149 Compare August 13, 2025 12:10
@compiler-errors
Copy link
Member

@bors r+ rollup=never

@bors
Copy link
Collaborator

bors commented Aug 13, 2025

📌 Commit 4d84149 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 13, 2025
@lcnr
Copy link
Contributor Author

lcnr commented Aug 18, 2025

let's check whether this can be rolled up instead. The bors queue is very long.

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

rust-bors bot added a commit that referenced this pull request Aug 18, 2025
actually provide the correct args to coroutine witnesses
@rust-bors

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 18, 2025
@rust-bors
Copy link

rust-bors bot commented Aug 18, 2025

☀️ Try build successful (CI)
Build commit: e02b440 (e02b440f79527faea218853ba71bbd61974eed15, parent: 425a9c0a0e365c0b8c6cfd00c2ded83a73bed9a0)

@rust-timer

This comment has been minimized.

@lqd
Copy link
Member

lqd commented Aug 18, 2025

We could just bump its priority above rollups :)

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e02b440): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
0.8% [0.8%, 0.8%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.1% [-0.1%, -0.1%] 1
All ❌✅ (primary) 0.8% [0.8%, 0.8%] 1

Max RSS (memory usage)

Results (secondary 4.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
4.7% [4.7%, 4.7%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

Results (primary -2.7%, secondary -9.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
5.3% [5.3%, 5.3%] 1
Improvements ✅
(primary)
-2.7% [-2.7%, -2.7%] 1
Improvements ✅
(secondary)
-13.0% [-16.7%, -2.2%] 4
All ❌✅ (primary) -2.7% [-2.7%, -2.7%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 470.373s -> 470.202s (-0.04%)
Artifact size: 377.75 MiB -> 377.70 MiB (-0.01%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 18, 2025
@lcnr
Copy link
Contributor Author

lcnr commented Aug 19, 2025

the perf impact looks spurious (the only change is in codegen)

@bors rollup=always

bors added a commit that referenced this pull request Aug 19, 2025
[BETA] Revert "Remove the witness type from coroutine args"

fixes #145151 and #145288

we do not revert on nightly as its instead fixed by #145194 and #145338.

See the discussion in https://rust-lang.zulipchat.com/#narrow/channel/238009-t-compiler.2Fmeetings/topic/.5Bweekly.5D.202025-08-14/near/534490313
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Aug 19, 2025
…mpiler-errors

actually provide the correct args to coroutine witnesses

rust-lang#145194 accidentally provided all arguments of the closure to the witness, but the witness only takes the generic parameters of the defining scope: https://github.com/rust-lang/rust/blob/216cdb7b22b637cef75b7225c642cb7587192643/compiler/rustc_hir_typeck/src/closure.rs#L164

Fixes rust-lang#145288
bors added a commit that referenced this pull request Aug 19, 2025
Rollup of 15 pull requests

Successful merges:

 - #145338 (actually provide the correct args to coroutine witnesses)
 - #145429 (Couple of codegen_fn_attrs improvements)
 - #145452 (Do not strip binaries in bootstrap everytime if they are unchanged)
 - #145464 (Stabilize `const_pathbuf_osstring_new` feature)
 - #145474 (Properly recover from parenthesized use-bounds (precise capturing lists) plus small cleanups)
 - #145486 (Fix `unicode_data.rs` mention message)
 - #145490 (Trace some basic I/O operations in bootstrap)
 - #145493 (remove `should_render` in `PrintAttribute` derive)
 - #145500 (Port must_use to the new target checking)
 - #145505 (Simplify span caches)
 - #145510 (Visit and print async_fut local for async drop.)
 - #145511 (Rust build fails on OpenBSD after using file_lock feature)
 - #145532 (resolve: debug for block module)
 - #145533 (Reorder `lto` options from most to least optimizing)
 - #145537 (Do not consider a `T: !Sized` candidate to satisfy a `T: !MetaSized` obligation.)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 99de64b into rust-lang:master Aug 19, 2025
11 checks passed
@rustbot rustbot added this to the 1.91.0 milestone Aug 19, 2025
rust-timer added a commit that referenced this pull request Aug 19, 2025
Rollup merge of #145338 - lcnr:coroutine-witness-yikes, r=compiler-errors

actually provide the correct args to coroutine witnesses

#145194 accidentally provided all arguments of the closure to the witness, but the witness only takes the generic parameters of the defining scope: https://github.com/rust-lang/rust/blob/216cdb7b22b637cef75b7225c642cb7587192643/compiler/rustc_hir_typeck/src/closure.rs#L164

Fixes #145288
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Aug 20, 2025
Rollup of 15 pull requests

Successful merges:

 - rust-lang/rust#145338 (actually provide the correct args to coroutine witnesses)
 - rust-lang/rust#145429 (Couple of codegen_fn_attrs improvements)
 - rust-lang/rust#145452 (Do not strip binaries in bootstrap everytime if they are unchanged)
 - rust-lang/rust#145464 (Stabilize `const_pathbuf_osstring_new` feature)
 - rust-lang/rust#145474 (Properly recover from parenthesized use-bounds (precise capturing lists) plus small cleanups)
 - rust-lang/rust#145486 (Fix `unicode_data.rs` mention message)
 - rust-lang/rust#145490 (Trace some basic I/O operations in bootstrap)
 - rust-lang/rust#145493 (remove `should_render` in `PrintAttribute` derive)
 - rust-lang/rust#145500 (Port must_use to the new target checking)
 - rust-lang/rust#145505 (Simplify span caches)
 - rust-lang/rust#145510 (Visit and print async_fut local for async drop.)
 - rust-lang/rust#145511 (Rust build fails on OpenBSD after using file_lock feature)
 - rust-lang/rust#145532 (resolve: debug for block module)
 - rust-lang/rust#145533 (Reorder `lto` options from most to least optimizing)
 - rust-lang/rust#145537 (Do not consider a `T: !Sized` candidate to satisfy a `T: !MetaSized` obligation.)

r? `@ghost`
`@rustbot` modify labels: rollup
@lcnr lcnr deleted the coroutine-witness-yikes branch August 20, 2025 09:18
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request Aug 20, 2025
Rollup of 15 pull requests

Successful merges:

 - rust-lang#145338 (actually provide the correct args to coroutine witnesses)
 - rust-lang#145429 (Couple of codegen_fn_attrs improvements)
 - rust-lang#145452 (Do not strip binaries in bootstrap everytime if they are unchanged)
 - rust-lang#145464 (Stabilize `const_pathbuf_osstring_new` feature)
 - rust-lang#145474 (Properly recover from parenthesized use-bounds (precise capturing lists) plus small cleanups)
 - rust-lang#145486 (Fix `unicode_data.rs` mention message)
 - rust-lang#145490 (Trace some basic I/O operations in bootstrap)
 - rust-lang#145493 (remove `should_render` in `PrintAttribute` derive)
 - rust-lang#145500 (Port must_use to the new target checking)
 - rust-lang#145505 (Simplify span caches)
 - rust-lang#145510 (Visit and print async_fut local for async drop.)
 - rust-lang#145511 (Rust build fails on OpenBSD after using file_lock feature)
 - rust-lang#145532 (resolve: debug for block module)
 - rust-lang#145533 (Reorder `lto` options from most to least optimizing)
 - rust-lang#145537 (Do not consider a `T: !Sized` candidate to satisfy a `T: !MetaSized` obligation.)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sqlx-postgres Fails to Build on Latest 1.91.0
8 participants